[monitoring] fix: use retrying module in the fixture class by tmatsuo · Pull Request #3285 · GoogleCloudPlatform/python-docs-samples

tmatsuo · 2020-04-06T22:42:47Z

fixes #2971
fixes #2972
fixes #2973
fixes #3085

It will likely fix those issues, not guaranteed, but it's worth a try.

fixes GoogleCloudPlatform#2971 fixes GoogleCloudPlatform#2972 fixes GoogleCloudPlatform#2973 fixes GoogleCloudPlatform#3085 It will likely fix those issues, not guaranteed, but it's worth a try.

kurtisvg · 2020-04-06T22:55:25Z

@@ -1 +1,3 @@
 pytest==5.3.2
+gcp-devrel-py-tools==0.0.15
+google-cloud-core


Any reason not to pin a version?

This is a temporary workaround. It'll go away.
(Actually, I'll use flaky, so those 2 lines will go away)

kurtisvg · 2020-04-06T23:03:23Z

 import random
 import string

+from gcp_devrel.testing import eventually_consistent


In general I'd prefer we stop relying on this module altogether. It's not maintained and a lot of the logic isn't really great for testing.

IIRC, eventually_consistent just attempts the function until it passes. This could make the issue of failures during concurrent tests worse. There are other libraries (like flaky) that do this better and don't add extra maintenance on us.

@kurtisvg
Flaky sounds like a plan. I'll use flaky for this test.

But, I kinda disagree with your two points; for the first point "It's not maintained", we can maintain it, right? I'm happy to do that from now on. For the second point, "lot of the logic isn't really great for testing." Can you elaborate? Maybe you can file a bug on the repo?

I still think eventually_consistent is good for eventually consistent tests. It is basically a wrapper around retrying module with default exponential backoff parameters.

But I agree that flaky is more suitable with this test.

RE: maintenance - it doesn't seem worth the effort of us maintaining and releasing a special module just for testing our samples. Are our libraries really so hard to test that we can't find an out of the box solution to reach reasonable tests? (I think the answer is no)

RE: logic - IMO, flakey tests are bad tests. Running a test several times hoping at least one passes means that potentially, our users also have to run the sample multiple times to make sure that it's working. Ideally, the snippet should work the first time, every time.

@kurtisvg Thanks for the discussion. Let's discuss further off PR :)

tmatsuo · 2020-04-06T23:51:43Z

@kurtisvg Thanks, now it's using flaky. PTAL

tmatsuo · 2020-04-07T00:05:19Z

Ah, as far as I read the logs correctly, flaky doesn't retry.

Oh I just found the pytest marker. Another commit is on the way.

tmatsuo · 2020-04-07T00:14:02Z

Oh I think I started to understand the error better.
The log says:

ERROR at setup of test_list_alert_policies

This is failing in the fixture setup. I think I'm going to retry the setup with retrying module with an exponential backoffs.

kurtisvg · 2020-04-07T00:17:08Z

@kurtisvg Thanks, now it's using flaky. PTAL

@tmatsuo You may also want to want to to try a retry filter with a pause. Something like this:

def retry_on_concurrent_edits(err, *args):
    time.sleep(1)
    return "Too many concurrent edits to the project configuration" in str(err)

@flaky(rerun_filter=delay_rerun)
def test():

I'm not sure if fixtures are recreated on retry, but that should at least give a pause in between different attempts (and only retry if it's the 409 error).

tmatsuo · 2020-04-07T00:26:52Z

@kurtisvg Now I use retrying module in the fixture class.

I hope it will fix the concurrent tests. Let's see the kokoro result.

tmatsuo · 2020-04-07T00:30:29Z

Ah, interesting. The latest retrying module doesn't support the tuple. I'll make a fix.

tmatsuo · 2020-04-07T00:42:12Z

Alright, the tests are now passing. Previously, one of the tests was constantly failing, so it's certainly an improvement. I'll re-run the tests few times to see how stable they are.

Note: this is not an ultimate fix because when we have more and more builds, it will cause problems. Also, the presubmit builds might conflict with continuous builds, etc etc.

I'll file another issue for the fundamental fix.

@kurtisvg PTAL

tmatsuo · 2020-04-07T01:04:46Z

Tests were all green 3 times in a row, but now we have another faiulre.

It seems like we need to retry in the test itself, and the teardown. Another commit is on the way.

tmatsuo · 2020-04-07T02:53:16Z

Tests are all green 5 times in a row.
@leahecole PTAL

leahecole

oh awesome love that we're moving to retry instead

fix: mark tests with @eventually_consistent.mark

ac2f9b0

fixes GoogleCloudPlatform#2971 fixes GoogleCloudPlatform#2972 fixes GoogleCloudPlatform#2973 fixes GoogleCloudPlatform#3085 It will likely fix those issues, not guaranteed, but it's worth a try.

tmatsuo requested review from kurtisvg and leahecole April 6, 2020 22:42

tmatsuo requested a review from a team as a code owner April 6, 2020 22:42

blunderbuss-gcf Bot assigned crwilcox Apr 6, 2020

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 6, 2020

kurtisvg suggested changes Apr 6, 2020

View reviewed changes

use flaky instead

cdb488f

tmatsuo changed the title ~~fix: mark tests with @eventually_consistent.mark~~ fix: mark tests with @flaky Apr 6, 2020

use pytest marker

c3edce4

tmatsuo changed the title ~~fix: mark tests with @flaky~~ fix: mark tests with @pytest.mark.flaky Apr 7, 2020

use retrying module in the fixture class

a78c2e6

tmatsuo changed the title ~~fix: mark tests with @pytest.mark.flaky~~ fix: use retrying module in the fixture class Apr 7, 2020

pass callback for retry_on_exception

5532261

kurtisvg approved these changes Apr 7, 2020

View reviewed changes